Expand Salesforce sync to SchoolClass, ClassTeacher, and Lesson#851
Conversation
Test coverage91.46% line coverage reported by SimpleCov. |
The Heroku Connect parent-sync race guard introduced for RoleSyncJob in #850 applies to every sync job that uses __r__ external-ID lookups to resolve a Salesforce parent. Move it onto the base class so SchoolClass, ClassTeacher, and Lesson sync jobs (added in this branch) can call it without duplicating the helper or its rationale. No behaviour change: RoleSyncJob still raises the same SalesforceRecordNotFound and is still retried by the base-class retry_on; the method now just lives one inheritance hop away. Co-authored-by: Cursor <cursoragent@cursor.com>
These ActiveRecord models map onto the Heroku Connect-managed tables (classroom__c, lesson__c, contact_classroom_affiliation__c) in the salesforce_connect database. Each declares its external-ID field, hides the Heroku-Connect-only sfid / _hc_lastop / _hc_err columns, and exposes Salesforce field accessors derived from the editor-api parent record so sync jobs can assemble payloads without leaking column names. Factories included for use in the upcoming sync-job specs. Pure data layer; nothing in the app references these models yet. Co-authored-by: Cursor <cursoragent@cursor.com>
Three new GoodJob-backed sync jobs follow the established SalesforceSyncJob pattern (mapped attributes + truncation + concurrency keyed on the source record id): Salesforce::SchoolClassSyncJob -> Classroom__c Salesforce::ClassTeacherSyncJob -> Contact_Classroom_Affiliation__c Salesforce::LessonSyncJob -> Lesson__c Each calls ensure_parent_synced! before saving so the new __r__ external-ID lookups can't permanently FAIL on a parent that hasn't landed yet; the base class retry handles redelivery. Adds matching rake tasks (salesforce_sync:school_class, salesforce_sync:class_teacher, salesforce_sync:lesson) so backfill stays consistent with the existing school/role/contact pattern. Includes a shared not_have_enqueued_job negated matcher in spec/support for use by the upcoming callback specs. No model callbacks yet -- jobs are wired up in the next commit. Co-authored-by: Cursor <cursoragent@cursor.com>
Hook the new sync jobs onto the editor-api model graph. Each callback is
guarded by FeatureFlags.salesforce_sync? so dev/test stay quiet by
default.
SchoolClass -> SchoolClassSyncJob on create/update
Lesson -> LessonSyncJob on create/update (class-bound)
ClassTeacher -> ClassTeacherSyncJob on create;
SchoolClassSyncJob on create/destroy
(refreshes numberofmembers__c)
ClassStudent -> SchoolClassSyncJob on create/destroy
(numberofmembers__c only -- no per-lesson
cascade; remix Project creation drives
lesson syncs instead)
Project (remix) -> LessonSyncJob on create
(numberofassignedprojects__c is exactly
remixes.count, which only changes here)
SchoolProject (state) -> recalculate submitted_projects_count and
enqueue LessonSyncJob on transitions
into/out of :submitted and :complete
Brand-new classes race SchoolClassSyncJob vs. ClassTeacherSyncJob; the
race is harmless because ensure_parent_synced! defers the child write
until the parent lands and per-record concurrency on the base job
collapses duplicate enqueues.
Includes model specs (school_class, lesson, class_teacher,
class_student, project) and the negated_matchers helper to assert
"on enqueue X and explicitly not Y" with compound expectations.
Co-authored-by: Cursor <cursoragent@cursor.com>
Scratch projects are the primary focus of this Salesforce work but they
don't transition through the SchoolProject state machine -- Experience
CS marks them complete by flipping school_projects.finished via
Concept::SchoolProject::SetFinished. Without a separate path the
Statesman-driven submitted_projects_count would miss every Scratch
completion and Lesson__c.numberofcompletedprojects__c would be 0 for
any Scratch-only lesson.
* Lesson#finished_projects_count: live read of school_projects.where(
finished: true).count. Not cached because its only consumer is the
per-lesson concurrency-limited LessonSyncJob; the recalc-on-write
machinery used for submitted_projects_count would be overkill here.
* LessonSyncJob: numberofcompletedprojects__c now sums
submitted_projects_count + finished_projects_count. The two flows
are mutually exclusive in practice, so a plain sum is safe.
* SchoolProject after_commit on saved_change_to_finished?: enqueue
LessonSyncJob so the Salesforce mirror updates when Experience CS
flips the flag. Statesman-driven transitions remain handled by the
state-machine after_transition callback wired in the previous
commit.
Recalculation of submitted_projects_count is unchanged: it still counts
only Statesman :submitted rows, keeping the two flows independent.
Co-authored-by: Cursor <cursoragent@cursor.com>
Capture the operational shape of the sync layer for future agents: where data goes (salesforce_connect DB, not the Salesforce API), how it's gated (SALESFORCE_ENABLED), the backfill ordering, and -- most importantly -- the parent-sync race guard that every job using __r__ external-ID lookups has to call before writing. Heroku Connect's permanent-FAIL behaviour on missing parents is a non-obvious failure mode; the doc names ensure_parent_synced!, points at the retry policy on the base class, and lists the call sites so the pattern stays consistent as more sync jobs are added. Co-authored-by: Cursor <cursoragent@cursor.com>
0625d55 to
f57968b
Compare
There was a problem hiding this comment.
Pull request overview
Extends the existing Heroku Connect–backed Salesforce sync layer to cover classroom-related entities (SchoolClass/ClassTeacher/Lesson) and adds event-driven triggers so Salesforce mirror rows stay up-to-date as roster, assignment, and completion signals change.
Changes:
- Added new Salesforce mirror models and sync jobs for
SchoolClass,ClassTeacher, and classroom-boundLesson, with parent-sync race protection viaSalesforceSyncJob#ensure_parent_synced!. - Added/updated model callbacks and event triggers (Project remix creation, SchoolProject finished changes, classroom roster changes) to enqueue the appropriate sync jobs.
- Added rake tasks, factories, and RSpec coverage for the new sync surface (including parent-not-synced retry behavior), plus small test helper support for compound negated matchers.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| spec/support/negated_matchers.rb | Adds a negated job-enqueue matcher for compound RSpec expectations. |
| spec/models/school_project_salesforce_sync_spec.rb | Verifies Lesson sync job enqueued when SchoolProject.finished flips. |
| spec/models/school_class_spec.rb | Adds model-level expectations for SchoolClass Salesforce sync enqueues. |
| spec/models/project_salesforce_sync_spec.rb | Verifies Lesson sync job enqueued on remix Project creation. |
| spec/models/lesson_spec.rb | Adds specs for finished_projects_count and Lesson Salesforce sync enqueue behavior. |
| spec/models/class_teacher_salesforce_sync_spec.rb | Verifies enqueues for classroom + affiliation sync on create/destroy. |
| spec/models/class_student_salesforce_sync_spec.rb | Verifies roster change enqueues classroom sync but not lesson sync. |
| spec/jobs/salesforce/school_class_sync_job_spec.rb | Covers field mappings, member count, retry guard, and disablement for SchoolClass sync. |
| spec/jobs/salesforce/lesson_sync_job_spec.rb | Covers Lesson field mappings, assigned/completed counts, retry guard, and disablement. |
| spec/jobs/salesforce/class_teacher_sync_job_spec.rb | Covers affiliation mappings, retry guard for both parents, and disablement. |
| spec/factories/salesforce/school_class.rb | Adds FactoryBot factory for Salesforce SchoolClass mirror rows. |
| spec/factories/salesforce/lesson.rb | Adds FactoryBot factory for Salesforce Lesson mirror rows. |
| spec/factories/salesforce/class_teacher.rb | Adds FactoryBot factory for Salesforce ClassTeacher mirror rows. |
| lib/tasks/salesforce_sync.rake | Adds backfill tasks for school classes, class teachers, and classroom lessons. |
| app/state_machines/school_project_state_machine.rb | Adds Salesforce Lesson sync enqueues on certain state transitions. |
| app/models/school_project.rb | Enqueues Lesson sync when Experience CS flips finished. |
| app/models/school_class.rb | Enqueues SchoolClass sync on create/update. |
| app/models/salesforce/school_class.rb | Adds AR model for salesforce.classroom__c. |
| app/models/salesforce/lesson.rb | Adds AR model for salesforce.lesson__c. |
| app/models/salesforce/class_teacher.rb | Adds AR model for salesforce.contact_classroom_affiliation__c. |
| app/models/project.rb | Enqueues Lesson sync when a remix is created (assignment count freshness). |
| app/models/lesson.rb | Adds classroom-only after_commit sync and finished_projects_count. |
| app/models/class_teacher.rb | Enqueues classroom sync on create/destroy and affiliation sync on create. |
| app/models/class_student.rb | Enqueues classroom sync on roster create/destroy. |
| app/jobs/salesforce/school_class_sync_job.rb | Implements SchoolClass mirror upsert with parent-sync guard. |
| app/jobs/salesforce/salesforce_sync_job.rb | Generalizes parent-sync race guard helper into base class. |
| app/jobs/salesforce/role_sync_job.rb | Removes now-redundant local parent-sync guard implementation. |
| app/jobs/salesforce/lesson_sync_job.rb | Implements Lesson mirror upsert incl. assigned/completed counts + parent guard. |
| app/jobs/salesforce/class_teacher_sync_job.rb | Implements ClassTeacher mirror upsert with dual parent guards. |
| AGENTS.md | Documents Salesforce sync operational expectations and backfill ordering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Counts Experience CS Scratch completions for this lesson. Experience CS marks a | ||
| # project complete by flipping school_projects.finished via Concept::SchoolProject::SetFinished, | ||
| # bypassing the Statesman state machine entirely — so submitted_projects_count never | ||
| # sees these. Read live (no cached column) because the only consumer is LessonSyncJob, | ||
| # which is per-lesson concurrency-limited; if a non-sync reader ever needs this, add a | ||
| # cached column then. Summed with submitted_projects_count in LessonSyncJob to derive | ||
| # Lesson__c.numberofcompletedprojects__c. | ||
| def finished_projects_count |
There was a problem hiding this comment.
I think this comment is a bit of an information overload and not sure how helpful it is.
- why would a non-sync reader need to use a cached column? I would expect this to be doing a single database query, what's the problem with that?
- I think it's unusual to list callers within documentation - if this changes then the comments not going to be kept in date.
Instead of this comment, I wondered if there's a way to make it clearer that this is for experience CS projects - for example just calling it finished_experience_cs_scratch_project_count would make that clear and maybe make the comment uneeded.
There was a problem hiding this comment.
I did consider putting some reference to experience_cs in the method name, but decided against it on the grounds that finished is a first-class schema field in editor-api, which leads to a bit of necessary explanation in the comment (I agree that it can be simplified and I will).
Let me know what you think - I'm happy to use your function name if you think it will be clearer going forward.
| mapped_attributes(lesson:).merge( | ||
| teacherprojecttitle__c: lesson.project&.name, | ||
| teacherprojecttype__c: lesson.project&.project_type, | ||
| numberofassignedprojects__c: lesson.remixes.count, |
There was a problem hiding this comment.
Is counting remixes a good way to tell us assigned projects?
Remixes count might be a good approximation for started projects, but it wouldn't tell you if a teacher has assigned a project. As far as I know we don't model project assignment, we just know if a project has been made available to a student in a class.
This might be a naming thing - maybe numberofassignedprojects__c should be called 'number of started/saved projects' or similar
There was a problem hiding this comment.
Yeah, I think it's a terminology issue, maybe. The assumption is that if the project is assigned to the class, it is assigned to all the students in the class.
There was a problem hiding this comment.
So maybe it could be removed if the existing class member count is sufficient?
There was a problem hiding this comment.
Apologies, I mis-remembered how that part worked. What counts as an "assigned" project is when the student has first saved their remix of a teacher's project.
There was a problem hiding this comment.
I saw you added a comment to the code, but I'm unsure about the reasoning behind this - what do the salesforce team want to track - project assignments or project starts?
If it's project assignments, shouldn't use the class member count (or remove this since we already have that?). if it's project starts, could this be renamed? Or is this terminology already used for reporting Experience CS metrics?
There was a problem hiding this comment.
We discussed this on Slack but, for the GitHub record, we decided that the actual metric is "projects visible to students in the classroom" so in commit aca52ff we changed it to this calculation.
|
I'm finding some of the comments in app/models files hard to understand, I think it's because they are terminology that's not in our codebase already (like 'roster', or 'parent classroom mirror'). I think it's also because they leak some of the details out of the salesforce implementation into the models. Do you think these comments are important? I wasn't sure if you've taken the time to write them or they were autogenerated. Some of these might be better documented in the PR or commit history. |
Drop the redundant `enqueue_salesforce_lesson_sync` after_transition callbacks on `SchoolProjectStateMachine`. State-machine transitions that change a synced field already fan out via `Lesson#recalculate_submitted_projects_count!` → `Lesson#after_commit :do_salesforce_sync`, so the explicit enqueues either duplicated existing jobs (collapsed by per-record concurrency) or were no-ops for transitions where nothing synced changes. Lock the new behaviour with specs covering both the "exactly once" path (via the recalc) and the "don't enqueue" path (unsubmitted ↔ complete). Rewrite hook comments on `ClassTeacher`, `ClassStudent`, `Project`, `SchoolProject`, and `Lesson` in codebase terms — drop "mirror", "roster", and `__c` field names that leaked Heroku-Connect / Salesforce vocabulary into model files. Add a one-line note at the `numberofassignedprojects__c` call site flagging the SF field-name mismatch (counts started, not assigned). Co-authored-by: Cursor <cursoragent@cursor.com>
|
On the question of leaking Salesforce into the models, I somewhat agree. One of my concerns here, that I rather ran out of time to consider doing, is that this work layers a network of event-driven sync jobs, where the dependencies between them might be rather non-obvious to future developers. |
…count Replace the remix-based approximation with the new semantic: a lesson is "assigned" to every student in its class iff its visibility is set to 'students'. The SF field name now matches its meaning, resolving the naming concern raised in PR #851 review. Reshape the sync triggers around the new dependency graph: - LessonSyncJob computes `lesson.school_class.students.count` when visibility == 'students', else 0. - ClassStudent create/destroy fans out to every visible-to-students lesson in the class. The fanout queries via the FK (`Lesson.where(school_class_id: ...)`) rather than the `school_class` association so it stays safe under cascading destroy: when a SchoolClass is destroyed, its students cascade and the after_commit fires after the parent row is gone, at which point the association resolves to nil. The FK on the in-memory destroyed record stays readable, and the lessons it once owned are nullified or deleted, so the relation is empty in the cascade case and correct otherwise. - Project#after_commit on remix create is removed — remix creation no longer affects any synced field on Lesson__c (numberofcompletedprojects__c is driven by SchoolProject state/finished changes; numberofassignedprojects__c no longer depends on remix count). Backfill: run `rails salesforce_sync:lesson` after deploy to refresh existing rows with the new count. Trade-off accepted: roster changes now enqueue N+1 jobs per ClassStudent event (one SchoolClassSyncJob plus one LessonSyncJob per student-visible lesson). Bounded fanout, collapsed by per-record concurrency on the base job for rapid duplicates. Loss to flag: Salesforce no longer carries a "students who have started" signal. If reporting wants that back, the cleanest path is a separate SF field fed by a restored Project#after_commit on remix create. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Great, thanks for the improvements |
Related to https://github.com/RaspberryPiFoundation/experience-cs/issues/2142
Summary
Extends the Heroku Connect Salesforce sync pattern from #677 to three more Salesforce objects:
SchoolClassClassroom__cEditor__clookupClassTeacherContact_Classroom_Affiliation__cClassroom__candContactlookupsLesson(classroom-bound)Lesson__cClassroom__clookupIt also generalises the parent-sync race guard that #850 introduced for
RoleSyncJobso every sync job using__r__<external_id>lookups gets it for free, and documents the operational shape of the sync layer inAGENTS.md.Behaviour
Triggers.
after_commiton each source model enqueues a per-record GoodJob (Salesforce::{SchoolClass,Lesson,ClassTeacher}SyncJob), gated onFeatureFlags.salesforce_sync?. Per-record concurrency on the base job collapses duplicate enqueues from same-transaction commits (e.g. new class + initial teacher).Lesson completion is two flows.
Lesson__c.numberofcompletedprojects__cis the sum of:Lesson#submitted_projects_count— Statesman-driven, used by the Code Editor (Python/HTML) flow when aSchoolProjecttransitions to:submitted. Unchanged from main.Lesson#finished_projects_count— live read ofschool_projects.where(finished: true).count, used by Experience CS for Scratch projects (which flipfinisheddirectly and never enter the state machine).The two flows are mutually exclusive in practice, so summing is safe.
SchoolProjectgets a newafter_commitonsaved_change_to_finished?to enqueueLessonSyncJobwhen Experience CS flips the flag.Assignment count is event-driven.
Lesson__c.numberofassignedprojects__cislesson.remixes.count, which only changes when a remixProjectis created. The enqueue lives onProject after_commit :create(remixed_from_id.present?), not on roster changes — so adding a student to a class no longer scans every lesson in the class.Parent-sync race guard.
ensure_parent_synced!(model, external_id_field, external_id, label)onSalesforce::SalesforceSyncJobchecks the parent has a non-nilsfidin its Heroku Connect mirror and raisesSalesforceRecordNotFoundif not. The base job retries with polynomial backoff, so jobs self-heal once the parent lands instead of leaving the child row permanentlyFAILEDin Heroku Connect. This is a generalisation of the mechanism introduced in #850.Backfill
After deploying with
SALESFORCE_ENABLED=true, run the backfill rake tasks in this order so each child's parent already has ansfidin its mirror by the time the child enqueues:Each task is
Model.find_each { perform_later }— non-blocking; the worker drains thesalesforce_syncqueue at concurrency 10. Watch progress at/admin/good_job.Strict ordering isn't load-bearing:
ensure_parent_synced!defers any child whose parent hasn't landed viaretry_on SalesforceRecordNotFound, wait: :polynomially_longer, attempts: 10, so out-of-order runs self-heal. Running parents first just cuts retry churn.Re-running is idempotent — each job uses
find_or_initialize_byon the external-ID key, so repeats update in place rather than duplicating rows.Out of scope
retry_on SalesforceRecordNotFoundexhausts its attempts.salesforce_sync:allconvenience task to run the canonical backfill order in one go.Test plan
docker compose run --rm api rspec spec/jobs/salesforce spec/models/{lesson,school_class,school_project_salesforce_sync,class_teacher_salesforce_sync,class_student_salesforce_sync,project_salesforce_sync}_spec.rbdocker compose run --rm api bundle exec rubocopSALESFORCE_ENABLED=true, run the backfill tasks per the Backfill section above.salesforce_connect, confirm every parent row has a non-nilsfidbefore child writes and that any transient_hc_errrows self-heal via the retry.Made with Cursor